compare XSRF token in constant time#10327
Conversation
|
|
||
| if (!expectedToken.equals(xsrfToken.getToken())) { | ||
| if (providedToken == null || !MessageDigest.isEqual( | ||
| expectedToken.getBytes(), providedToken.getBytes())) { |
There was a problem hiding this comment.
By all rights this should be ASCII, since at least the expectedToken will be from StringUtils.toHexString above, but nothing guarantees the incoming xsrf token will be in that charset. I don't think there's any way that can go wrong, since we know we're limited to [0-9A-F] on the first array to pass to isEqual, but just wanted to call it out in case anyone else spotted a problem there.
There was a problem hiding this comment.
Good point. Made both getBytes calls explicit UTF-8 so it no longer rides on the platform default charset. Expected token is hex so it is unaffected either way, but this keeps the provided side deterministic too.
|
@metsw24-max what tool did you use to find this? |
|
@zbynek no special tool, I was reading through the XSRF servlet and the String.equals on a secret-derived value jumped out as a timing oracle. I do lean on an LLM a bit for sanity-checking, but the find itself was just reading the code. |
niloc132
added
the
ready
3 participants
validateXsrfToken in XsrfProtectedServiceServlet checks the client-supplied token against the expected value (MD5 hex of the session cookie) with String.equals, which returns on the first mismatching character. The expected value is secret-derived and the check runs on every protected RPC call, so the per-character timing difference is an oracle a remote caller can use to recover the token and defeat the CSRF protection. Switch to MessageDigest.isEqual on the byte arrays so the comparison time no longer depends on how many leading characters match.